✨ Add Namespace Scoped Zone Discovery and Watch #3146

zhanggbj · 2024-08-08T08:31:12Z

What this PR does / why we need it:
This is to support the new Namespace Scoped Zone for isolation.

Introduce a feature flag to enable Namespace Scoped Zone.
Enhance zone discovery to support Namespace Scoped Zones.
Filter out zones marked for deletion during the discovery process.
Add Watch for Zone changes to update FailureDomains accordingly

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

sbueringer · 2024-08-08T10:04:23Z

@zhanggbj Should we already take a look or wait until the PR is out of draft?

zhanggbj · 2024-08-08T10:25:20Z

@sbueringer @chrischdi @fabriziopandini
Just removed the WIP as I was fixing some test job errors, but feel free to start a review, thanks!

About testing results with a real supervisor:

With a Supervisor which has both Cluster scoped AZ (zone-1, zone-2 and zone-3)and NS scoped Zone (zone-2 and zone-3), with NamespaceScopedZone enabled, cluster creation completes successfully.

tkg-test    tkg-multizone-e-drr4s-kpl8m                     zone-2   vsphere://302b8630-234d-4870-9ba6-0440fa2bf00e   192.168.128.77
tkg-test    tkg-multizone-e-drr4s-svwcc                     zone-3   vsphere://1c295476-e071-4d31-b737-7e30467650e3   192.168.128.74
tkg-test    tkg-multizone-e-drr4s-v7wzr                     zone-2   vsphere://ebb2f15d-5196-4dc7-88e5-fac9f6ced243   192.168.128.73
tkg-test    tkg-multizone-e-worker-np-1-2plnn-4jj56-f5jgl   zone-2   vsphere://cecc5158-90d8-4226-9af8-165d9d4f3f7c   192.168.128.76
tkg-test    tkg-multizone-e-worker-np-3-8kw7k-vnq7j-mw8p5   zone-3   vsphere://e1cda4ef-1826-4347-a9e0-740e61716237   192.168.128.75

With a Supervisor which has both Cluster scoped AZ (zone-1, zone-2 and zone-3)and NS scoped Zone (zone-2 and zone-3), with NamespaceScopedZone disabled, CAPV will discover Cluster scoped AZ zone-1, zone-2 and zone-3 as expected.

chrischdi

We also need to add a watch ca. at:
https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/main/controllers/vspherecluster_controller.go#L75

But only if the featuregate is enabled.

It should then use a EnqueueRequestFromMapFunc for all clusters in the namespace where the zone change was detected.

controllers/vmware/vspherecluster_reconciler.go

feature/feature.go

go.mod

external/tanzu-topology/api/v1alpha1/groupversion_info.go

controllers/vmware/vspherecluster_reconciler_test.go

external/tanzu-topology/api/v1alpha1/availability_zone.go

zhanggbj · 2024-08-09T07:26:15Z

We also need to add a watch ca. at: main/controllers/vspherecluster_controller.go#L75

But only if the featuregate is enabled.

It should then use a EnqueueRequestFromMapFunc for all clusters in the namespace where the zone change was detected.

Sure, we need to watch the Zone event, but I was planning to add it in a separate PR, as so far I cannot test it locally as the supervisor hasn't fully supported Zone deletion.

Currently only Zone discovery and cluster creation can be tested.

sbueringer · 2024-08-09T07:44:57Z

I really would prefer adding the watch in this PR already, even if we can't manually test it. Although it's not clear to me why we can't test it. Basically you can add/remove Zone objects and these should be then reflected in VSphereCluster and directly afterwards on Cluster, right?
(they don't even have to work to test that)

zhanggbj · 2024-08-09T07:46:16Z

Have to add a dot after this line, otherwise godot linter will fail Comment should end in a period (godot) and seems I cannot add a //nolint:godot in a commented line.

// SPDX-License-Identifier: Apache-2.0

zhanggbj · 2024-08-09T07:52:27Z

@chrischdi @sbueringer
I'm OK to add the Watch in this PR for code review first, I'll test it later locally, just want to make sure code changes are tested with the supervisor before sending for review.

chrischdi · 2024-08-09T08:18:05Z

Have to add a dot after this line, otherwise godot linter will fail Comment should end in a period (godot) and seems I cannot add a //nolint:godot in a commented line.
// SPDX-License-Identifier: Apache-2.0

Let's add a global ignore to this path in .golangci.yaml

Should then be something like:

  skip-files:
    - "zz_generated.*\\.go$"
    - "_conversion\\.go$"
    - "vendored_cluster_api\\.go$"
    - "^internal/apis/topology/v1alpha1"

controllers/vmware/vspherecluster_reconciler.go

k8s-ci-robot · 2024-08-13T04:24:17Z

LGTM label has been added.

Git tree hash: 73bebc55504d8679c600b53018c2cd01d0629ff6

sbueringer · 2024-08-13T04:24:26Z

/assign @chrischdi

for a final review

chrischdi

/approve
/hold cancel

k8s-ci-robot · 2024-08-13T08:32:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [chrischdi,sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sbueringer · 2024-08-13T08:33:23Z

/hold
for another round of tests on the mirror :)

- Introduce a feature flag to enable Namespace Scoped Zone. - Enhance zone discovery to support Namespace Scoped Zones. - Filter out zones marked for deletion during the discovery process.

Signed-off-by: Gong Zhang <[email protected]>

chrischdi · 2024-08-13T09:24:06Z

/lgtm

/test help

k8s-ci-robot · 2024-08-13T09:24:09Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main
/test pull-cluster-api-provider-vsphere-test-main
/test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-vsphere-apidiff-main
pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main

In response to this:

/lgtm

/test help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2024-08-13T09:24:11Z

LGTM label has been added.

Git tree hash: 09e4e29248e364c519a92078215bfb1640c9f861

chrischdi · 2024-08-13T09:24:16Z

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

chrischdi · 2024-08-13T09:25:04Z

/hold

Waiting for jobs here and at team-cluster-api#7 to succeed

sbueringer · 2024-08-13T12:50:19Z

Tests are green.

Nice work everyone!

/hold cancel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 8, 2024

k8s-ci-robot requested review from fabriziopandini and gab-satchi August 8, 2024 08:31

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 8, 2024

zhanggbj force-pushed the add_zone_ds branch from 250cdcd to 54fc6b0 Compare August 8, 2024 08:37

zhanggbj marked this pull request as draft August 8, 2024 08:41

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2024

zhanggbj force-pushed the add_zone_ds branch 3 times, most recently from 4247967 to e19a5cd Compare August 8, 2024 09:46

zhanggbj force-pushed the add_zone_ds branch from e19a5cd to 9a767a9 Compare August 8, 2024 10:15

zhanggbj marked this pull request as ready for review August 8, 2024 10:17

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2024

k8s-ci-robot requested review from chrischdi and rvanderp3 August 8, 2024 10:17

zhanggbj force-pushed the add_zone_ds branch from 9a767a9 to a9dfeee Compare August 8, 2024 10:30

chrischdi mentioned this pull request Aug 8, 2024

✨ Add Namespace Scoped Zone Discovery team-cluster-api/cluster-api-provider-vsphere#7

Closed

chrischdi reviewed Aug 8, 2024

View reviewed changes

external/tanzu-topology/api/v1alpha1/availability_zone.go Outdated Show resolved Hide resolved

zhanggbj force-pushed the add_zone_ds branch from a9dfeee to e20a2e7 Compare August 9, 2024 07:55

chrischdi reviewed Aug 9, 2024

View reviewed changes

controllers/vmware/vspherecluster_reconciler.go Outdated Show resolved Hide resolved

zhanggbj force-pushed the add_zone_ds branch 2 times, most recently from 404ed05 to 73e4950 Compare August 9, 2024 08:37

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2024

k8s-ci-robot assigned chrischdi Aug 13, 2024

chrischdi reviewed Aug 13, 2024

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2024

zhanggbj and others added 2 commits August 13, 2024 16:39

Add Namespace Scoped Zone Discovery

f1145e8

- Introduce a feature flag to enable Namespace Scoped Zone. - Enhance zone discovery to support Namespace Scoped Zones. - Filter out zones marked for deletion during the discovery process.

vspherecluster: re-write tests for getFailureDomains

1b14a58

zhanggbj force-pushed the add_zone_ds branch from ea27fb5 to 9752691 Compare August 13, 2024 08:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2024

k8s-ci-robot requested review from chrischdi and sbueringer August 13, 2024 08:42

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2024

Add Watch for Zone event to update FailureDomains accordingly

2afce2d

Signed-off-by: Gong Zhang <[email protected]>

zhanggbj force-pushed the add_zone_ds branch from 9752691 to 2afce2d Compare August 13, 2024 08:46

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2024

k8s-ci-robot merged commit 4f8d638 into kubernetes-sigs:main Aug 13, 2024
14 checks passed

k8s-ci-robot added this to the v1.11 milestone Aug 13, 2024

zhanggbj mentioned this pull request Oct 31, 2024

REQUEST: New membership for zhanggbj kubernetes/org#5237

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add Namespace Scoped Zone Discovery and Watch #3146

✨ Add Namespace Scoped Zone Discovery and Watch #3146

zhanggbj commented Aug 8, 2024 •

edited

Loading

sbueringer commented Aug 8, 2024

zhanggbj commented Aug 8, 2024 •

edited

Loading

chrischdi left a comment

zhanggbj commented Aug 9, 2024

sbueringer commented Aug 9, 2024 •

edited

Loading

zhanggbj commented Aug 9, 2024 •

edited

Loading

zhanggbj commented Aug 9, 2024 •

edited

Loading

chrischdi commented Aug 9, 2024

k8s-ci-robot commented Aug 13, 2024

sbueringer commented Aug 13, 2024 •

edited

Loading

chrischdi left a comment

k8s-ci-robot commented Aug 13, 2024

sbueringer commented Aug 13, 2024

chrischdi commented Aug 13, 2024

k8s-ci-robot commented Aug 13, 2024

k8s-ci-robot commented Aug 13, 2024

chrischdi commented Aug 13, 2024

chrischdi commented Aug 13, 2024

sbueringer commented Aug 13, 2024

✨ Add Namespace Scoped Zone Discovery and Watch #3146

✨ Add Namespace Scoped Zone Discovery and Watch #3146

Conversation

zhanggbj commented Aug 8, 2024 • edited Loading

sbueringer commented Aug 8, 2024

zhanggbj commented Aug 8, 2024 • edited Loading

chrischdi left a comment

Choose a reason for hiding this comment

zhanggbj commented Aug 9, 2024

sbueringer commented Aug 9, 2024 • edited Loading

zhanggbj commented Aug 9, 2024 • edited Loading

zhanggbj commented Aug 9, 2024 • edited Loading

chrischdi commented Aug 9, 2024

k8s-ci-robot commented Aug 13, 2024

sbueringer commented Aug 13, 2024 • edited Loading

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 13, 2024

sbueringer commented Aug 13, 2024

chrischdi commented Aug 13, 2024

k8s-ci-robot commented Aug 13, 2024

k8s-ci-robot commented Aug 13, 2024

chrischdi commented Aug 13, 2024

chrischdi commented Aug 13, 2024

sbueringer commented Aug 13, 2024

zhanggbj commented Aug 8, 2024 •

edited

Loading

zhanggbj commented Aug 8, 2024 •

edited

Loading

sbueringer commented Aug 9, 2024 •

edited

Loading

zhanggbj commented Aug 9, 2024 •

edited

Loading

zhanggbj commented Aug 9, 2024 •

edited

Loading

sbueringer commented Aug 13, 2024 •

edited

Loading